Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wallet history details #1054

Merged
merged 1 commit into from
Aug 15, 2023
Merged

Wallet history details #1054

merged 1 commit into from
Aug 15, 2023

Conversation

Restioson
Copy link
Contributor

@Restioson Restioson commented Aug 2, 2023

I'm opening this as a draft since I want a second opinion on the type of info to show in the expanded view before I put more work into styling it.

In list:
image

When tapped:
image

Possibilities:

  • Add "Lighting" or "On-chain" to the title (it is displayed on the right hand side, though)
  • Truncate the payment hash
  • If it's an on-chain payment, link to a block explorer for that txid
  • Change date format
  • other info to add?

Fixes #1011, and is a start on #872

@Restioson Restioson requested review from holzeis and luckysori August 2, 2023 14:24
@Restioson Restioson linked an issue Aug 2, 2023 that may be closed by this pull request
6 tasks
@Restioson
Copy link
Contributor Author

Restioson commented Aug 2, 2023

I wonder why lint-flutter and ln-dlc-ode-tests is failing on this 🤔 I didn't touch anything in native, and lint-flutter is failing to install some binding thing which I also didn't touch. Possibly related to #1049?

@Restioson
Copy link
Contributor Author

Restioson commented Aug 2, 2023

Possibly related: #982. May or may not be worth a follow-up PR.

@holzeis
Copy link
Contributor

holzeis commented Aug 3, 2023

I like the proposal of expandable tiles 👍.

Did you also try how it would look with a details screen? In terms of a pattern it would probably allow us to show more complex data as well.

@luckysori
Copy link
Contributor

I'm opening this as a draft since I want a second opinion on the type of info to show in the expanded view before I put more work into styling it.

In terms of information to display, here are some suggestions:

On-chain transaction

  • Title: On-chain transaction (means we could remove the little on-chain/off-chain tag).
  • Expanded view:
    • TXID.
    • Timestamp.
    • Number of confirmations (0 if pending!).
    • Button to jump to block explorer.

Off-chain payment

This would be for regular Lightning payments. At some point it would be nice to differentiate the JIT channel opening payment, but I'm not sure that's possible yet.

  • Title: Lightning payment.
  • Expanded view:
    • Payment hash (you can truncate it, but maybe we can have a button next to it to copy in full).
    • Status: completed, pending or failed.
    • Timestamp (I think this one corresponds to the last status update for this payment).

Trade

At the moment we can only open and close, but I think we should be talking about trades anyway.

  • Title: Trade (or open/close position for now).
  • Expanded view: we probably don't want to be redundant with the position view in the trade screen, so maybe we can just display timestamp, open/close position (if not in the title) and order ID.

JIT channel opening fee

  • Title: Channel-opening fee.
  • Expanded view: this could be the same as a regular Lightning payment. Maybe we can also identify the channel ID if the information is available. The only problem that I see is that if we get rid of the off-chain tag then it's not obvious that this was paid off-chain.

Order-matching fee

  • Title: Order-matching fee.
  • Expanded view: same as a regular Lightning payment, but with a link to the order ID.

These are only ideas, based on the original ticket and what you mentioned already.

@luckysori
Copy link
Contributor

I wonder why lint-flutter and ln-dlc-ode-tests is failing on this 🤔 I didn't touch anything in native, and lint-flutter is failing to install some binding thing which I also didn't touch.

ln-dlc-node-tests fails because of a flaky test, sorry! I need to check if we have an issue for that and look into it. It should work if you retry a few times (doesn't seem to fail all that often!).

lint-flutter failing is definitely weird! I don't know about that one. I wonder if it's transient although it doesn't look like it 🤔

Possibly related to #1049?

Hmmm, I don't think so. I've been able to merge things since that merged.

@Restioson
Copy link
Contributor Author

Restioson commented Aug 3, 2023

Did you also try how it would look with a details screen? In terms of a pattern it would probably allow us to show more complex data as well.

You mean in terms of opening a separate screen when you click on the tile? I haven't done that yet since there isn't enough data to show in the model. Maybe a "show more details" button could be added, though.

@Restioson
Copy link
Contributor Author

@luckysori thanks for all the suggestions! I'll go over a few:

Number of confirmations (0 if pending!).

Currently not represented in the model. Should this be done in a followup rather perhaps?

Status: completed, pending or failed.

I think we only show failed and pending in the view currently - this is already shown in the header, so is it worth to show it again?

JIT channel opening fee

Also not yet modelled

with a link to the order ID

A link in what sense? Where would it take you?


In general, I like these ideas! I do think it might make more sense to postpone them to a followup PR, though, since they require significant work on the modelling in Flutter. My proposal is for this PR to just introduce the UI elements and display all info we currently model, and then open a followup to model and display the rest. The modelling could be slightly more complex than just adding fields, since we might want to split it out into different classes instead of having just one. I really wish we had Rust enums in Dart...

@bonomat
Copy link
Contributor

bonomat commented Aug 4, 2023

You mean in terms of opening a separate screen when you click on the tile? I haven't done that yet since there isn't enough data to show in the model. Maybe a "show more details" button could be added, though.

I propose to go with a popup instead.
Something like Breez has:

image

I can design something simple in Figma first

@Restioson
Copy link
Contributor Author

I can design something simple in Figma first

Sure, I can work on the modal UI "infrastructure" until then and pivot away from the expandible tiles?

@Restioson
Copy link
Contributor Author

I've pushed my current changes to wallet-history-details-expansionpanel and I am going to undo the commits with the ExpansionPanel here

@bonomat
Copy link
Contributor

bonomat commented Aug 4, 2023

Sure, I can work on the modal UI "infrastructure" until then and pivot away from the expandible tiles?

Sure, please go ahead.

@Restioson
Copy link
Contributor Author

If we are going to use a dialogue, how do we communicate to the user that this is a clickable tile? There are some other places in the app like with the order screen which have hidden affordances like this. I.e, the user can do it, but it is not communicated by the UI. It just looks like an immutable tile to the user. With the dropdown arrow, it is clear that it can be expanded, but I'm not sure what we should do for a dialogue

@bonomat
Copy link
Contributor

bonomat commented Aug 4, 2023

If we are going to use a dialogue, how do we communicate to the user that this is a clickable tile?

I don't know about you, but I usually click everywhere in an app if I want to see more details :)

I've added a quick video to the ticket and the figma link: #872

@luckysori
Copy link
Contributor

@luckysori thanks for all the suggestions! I'll go over a few:

Number of confirmations (0 if pending!).

Currently not represented in the model. Should this be done in a followup rather perhaps?

Don't worry if it's not modeled. It was just a thought.

Status: completed, pending or failed.

this is already shown in the header, so is it worth to show it again?

Good point. It's probably better to only display it one way.

JIT channel opening fee

Also not yet modelled

I think you are right! We can treat it as a regular payment until we do model it properly. I think there's an open issue for that.

with a link to the order ID

A link in what sense? Where would it take you?

Sorry for the poor word choice. By "link" I meant that the order ID would be displayed in the detailed view.

In general, I like these ideas! I do think it might make more sense to postpone them to a followup PR, though, since they require significant work on the modelling in Flutter. My proposal is for this PR to just introduce the UI elements and display all info we currently model, and then open a followup to model and display the rest. The modelling could be slightly more complex than just adding fields, since we might want to split it out into different classes instead of having just one. I really wish we had Rust enums in Dart...

👍 Yep, it makes sense to not modify the model with this PR. We can consider what I wrote when we have a chance to expand the model.

@Restioson
Copy link
Contributor Author

I don't know about you, but I usually click everywhere in an app if I want to see more details :)

As do I, but I know that many users don't 😅 Anecdote, I once had someone complain to me that the notification button on our university's online platform was "useless" because it didn't tell you where the notifications came from (it would just say "Notification @ [time]". In actual fact, if you clicked on the notification line it would expand...

Hence, it might be better design to add some kind of indication that it can be clicked on. Maybe there's a standard in material design for that

@Restioson Restioson force-pushed the wallet-history-details branch from ab4e65e to 834a2ea Compare August 14, 2023 12:16
@Restioson
Copy link
Contributor Author

Thoughts?

image

@Restioson Restioson marked this pull request as ready for review August 14, 2023 12:16
Copy link
Contributor

@luckysori luckysori left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job, I think this is nice!

@@ -98,10 +98,13 @@ class WalletHistoryItem extends StatelessWidget {
}
}();

var amountFormatter = NumberFormat.compact(locale: "en_IN");
var amountFormatter = NumberFormat.compact(locale: "en_UK");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓Is the locale choice important and/or deliberate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is a small chore fix which I could extract. It's just weird that we use en_IN, and since Indian English has got lakh and other ways to abbreviate numbers I thought it could be slightly dangerous too


return Card(
child: ListTile(
onTap: () async {
await showDialog(context: context, builder: (ctx) => showItemDetails(title, ctx));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓What does showDialog do and where does it come from? I'm curious because I think our code is just showItemDetails, so this must be some kind of pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a flutter function to show the dialogue as a modal: https://api.flutter.dev/flutter/material/showDialog.html

mobile/lib/features/wallet/wallet_history_item.dart Outdated Show resolved Hide resolved
@Restioson Restioson force-pushed the wallet-history-details branch from 834a2ea to b7e13e8 Compare August 14, 2023 15:48
@Restioson
Copy link
Contributor Author

Restioson commented Aug 15, 2023

The Figma prototype looks really nice @bonomat! I'll merge this now (as it fixes #1011) and get on implementing the Figma after since it requires some more modelling first

@Restioson Restioson added this pull request to the merge queue Aug 15, 2023
Merged via the queue into main with commit 179119c Aug 15, 2023
@Restioson Restioson deleted the wallet-history-details branch August 15, 2023 14:48
@Restioson Restioson restored the wallet-history-details branch August 18, 2023 12:35
@Restioson Restioson deleted the wallet-history-details branch August 18, 2023 12:36
@Restioson
Copy link
Contributor Author

Accidental restoration 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transaction history text cut short
4 participants